Eh. Missed the fact that this member variable had been renamed between 2.5 and 2.6, and I committed the fix with 2.6 name to 2.5. Surprise – it didn't build. Fixed that now.

Thank you.
I didn't test this or anything, but IMO mappings shouldn't use a custom ed-of-track threshold but connect to [ChannelN],end_of_track (threshold set in Preferences ‣ Waveforms ‣ End of track warning) so the GUI and controller are in sync (not necessarily the blining but the timing)

@ronso0 Fair point, but that's a separate issue. The mapping itself is in need of a modernization. I have recently gotten a DDJ-SX and mixxx logged warnings becuse the mapping was still using connectControl instead of makeConnection. It has fallen behind in several other aspects too. I have fixed most of that already and I'm testing the updated mapping with the controller. Once I'm reasonably sure that I have not screwed anything up, I'll open a PR. So while I'm at it, I can also try adjusting the blink code so that it will take into account [ChannelN],end_of_track.

That sample counter is a parallel issue — the counter exists, but it starts off uninitialized (it's assigned only in setNewPlaypos) and is never reset. I think there's a fair chance that the respective code was simply lost during refactoring.

I agree to both.

Will take a look at this soonish.

Though CI fails (on some targets onl??) because the engine somehow relies on the current behavior?
Can you take a look at that, too?

Maybe we need to keep playposition with fast update and add a visual_playposition control which updates at 15 Hz?
(just a wild guess..)

visual_playposition is updated with the VSYNC of the main screen. This must be always in sync to prevent visual jitter.

Some CI targets indeed failed, and the pattern seems random. Almost as if it were some kind of a timing issue. 😁 I'll look into this.

Tests assume that a call to ProcessBuffer() will always update playposition. Before my change, this was indeed the case, and it was a direct consequence of not resetting the counter. The counter just kept incrementing, consequently it was always above the threshold, and playposition updated on every call. But now it doesn't - it updates every couple of calls (~15 Hz). The only thing that's slightly puzzling is why only some CI pipelines failed on the tests - because this behavior looks completely deterministic to me.

Not sure how to proceed. On one hand, it sounds sensible to keep playposition accurate, i.e. to not decimate. On the other hand, if we don't decimate, a mapping that connects to this CO will receive a lot of updates, potentially at a rate that (I imagine) can saturate the MIDI channel if every update is passed onwards. This is outside of my area of expertise. Please let me know your thoughts.

@alajovic you're right, playposition is updated on every process call¹, as well as CueControl::updateIndicatorsAndModifyPlay()

¹ corresponds to buffer size, or "engine process rate", which may be up to 750 Hz with minimal buffer of 1.33 ms
this may also explain why we didn't get bug reports for latencies used by most users (just my assumption) of 5-20 ms

Btw IIUC this also calls AutoDJProcessor::calculateTransition at engine rate, which may be a bit too much IMO

visual_playposition is updated with the VSYNC of the main screen.

VisualPlayposition is a class, there is no CO visual_playposition.
My suggestion was just to keep playposition updates as is (engine rate) and add a CO that updates with the desired rate of 15 Hz.

@alajovic as a quick fix for your mapping issue I suggest you move m_playposSlider->set(fFractionalPlaypos); out of the rate limiting if scope.
We need to check which consequences the rapid plapyos updates have, and which units actually rely on this fast update by no -- and which other units might suffer from it. (Woverview for example does not, it repaints only if the resulting pixel pos ahs changed, so fast updates are irrelevant even though the slot is called)

@daschuer maybe you can shed a light on this?

I can confirm that this is an issue and the fix looks good. However we need to fix the tests as well.

However we need to fix the tests as well.

This requires to either make tests wait for the same period, or add another CO that is always accurate.
The latter is appropriate IMO since VinylControlXwax::analyzeSamples() relies on the playpos to be accurate, right?

I looks like this is a regression in #1963
5a4444d in Mixxx 2.3

Accurate is relative in a multithreading environment. No one want to run another thread in 1 ms outside of the engine thread.

This means anything that was added after that might be assume the fast update rate and my suffer from a regression after merging that.

Ok, so you are right, we need to look at this in a bigger scope. Maybe not a 2.5 bug fix.

Thanks for your comments. If I understand correctly, some parts of the code may actually depend on playposition being updated with the engine rate. In such a case, I see two options:

1. Keep playposition as-is (updated at engine rate) and add a rate-limited CO named something like playposition_visual or playposition_display. Advantage: adding a new CO does not affect current behavior in any way. Disadvantage: all mappings that currently subscribe to playposition keep receiving the updates with engine rate (unless they are switched over to the new CO).

2. Rate-limit playposition and add a new CO, for example playposition_hires, that updates at full engine rate. Advantage: all mappings that currently subscribe to playpos get rate limiting for free (I wouldn't expect any mapping to need updates at engine rate). Disadvantage: parts of C++ code that require full resolution need to be switched over to the new CO.

playposition is used extensively by the mappings. In C++ code , the string "playposition" appears only 11 times, and 4 of those are in the tests. So option 2 doesn't seem too bad.

Yes, let's go for 2. But without a new CO.
Let me explain:

Using a CO with an down to 1000 Hz update rate is even an issue for the GUI thread delivering the update signals. There is also a even locking involved, unfortunately. So the removal of it here will be a major win for low latency setups.
So we should not reintroduce the same issue by a new CO.

From the threading theory, there can't also be a need by other asyncron tasks for updates that right rate. Assuming that processing in a non RT thread is anyway done suffering suspention by the RT thread advancing the play position behind its back. Because of this it can be always considered as "old". It does not matter how "old' it is for the workaround code.

For timed play position, we have already the visual play position code.

So we may keep using the 15 Hz update rate or refactor the code to use visual play position.

I have found these places in the code:

https://github.com/search?q=repo%3Amixxxdj%2Fmixxx+%5C%22playposition%5C%22+language%3AC%2B%2B&type=code

Productive code:

Open? Refactor to visual play position?

Visual, 15 Hz OK:

Visual, 15 Hz OK:

Used to set a cue point. Doing it while playing will use a delayed CU point. User is not that fast is OK.

Running Auto DJ with 15 Hz is desired.

Unittests:

Various unittest do a single Engine call and check the playosition.
They need to refactor to use:
EngineBuffer::m_playPos

Maybe similar to here:

Alterntive is to set
m_iSamplesSinceLastIndicatorUpdate = 1000000;

@jclsn can you also have a look and confirm?

I think we need to immediately after

m_pVisualPlayPos = VisualPlayPosition::getVisualPlayPosition(m_group);

    double playPosition;
    double tempoTrackSeconds;
    m_pVisualPlayPos->getTrackTime(&playPosition, &tempoTrackSeconds);

This can also be used for testing BTW.

@daschuer I honestly don't understand what this is about. I haven't read the vinylcontrolprocessor that closely.

@daschuer What about if we replace this:

    QSharedPointer<VisualPlayPosition> m_visualPlayPos;

and then in constructor

m_visualPlayPos(VisualPlayPosition::getVisualPlayPosition(group))

and then in analyzeSamples() just call m_visualPlayPos->getEnginePlayPos() instead of fetching playposition?

That would be a fairly minimal intervention. Do you see any potential problems with it?

That's a good solution. Go ahead.

I have implemented the suggested approach, please review. One pipeline check is failing, but it looks like it triggers on code that wasn't changed in the scope of my commits.

Since this can become a perfomance issue I have assigned it to the 2.6 milestone.
Please unassign if you don't agree and think it can wait for 2.6.1

Should I rebase onto 2.6 or is it fine if it stays as it is?